mysql-proxy: skip admin for no-FROM multi-expression selects; add tests#37391
Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!isUseDatabase && isMultiExpressionSelect(sqlStatement)) { | ||
| return Optional.empty(); | ||
| } | ||
| return isUseDatabase ? Optional.empty() : Optional.of(new UnicastResourceShowExecutor(sqlStatement, sql)); | ||
| } | ||
|
|
||
| private static boolean isMultiExpressionSelect(final SelectStatement sqlStatement) { |
There was a problem hiding this comment.
The method name isMultiExpressionSelect is ambiguous as it only checks if there are multiple projections, not specifically expressions. Consider renaming to hasMultipleProjections to more accurately reflect what the method validates.
| if (!isUseDatabase && isMultiExpressionSelect(sqlStatement)) { | |
| return Optional.empty(); | |
| } | |
| return isUseDatabase ? Optional.empty() : Optional.of(new UnicastResourceShowExecutor(sqlStatement, sql)); | |
| } | |
| private static boolean isMultiExpressionSelect(final SelectStatement sqlStatement) { | |
| if (!isUseDatabase && hasMultipleProjections(sqlStatement)) { | |
| return Optional.empty(); | |
| } | |
| return isUseDatabase ? Optional.empty() : Optional.of(new UnicastResourceShowExecutor(sqlStatement, sql)); | |
| } | |
| private static boolean hasMultipleProjections(final SelectStatement sqlStatement) { |
quanyeyang
force-pushed
the
fix-issue-37276
branch
from
5910e68 to
7a5bae5
Compare
There was a problem hiding this comment.
Thanks for the quick, targeted fix. It cleanly addresses #37276 by rerouting no-FROM multi-projection selects away from the admin path (root cause of the column-count mismatch) while keeping sysvar and special-function handling intact. The added tests cover both the regression and sysvar behavior. I don’t see regressions or compatibility risk; LGTM to merge. Nice work!
|
Thanks a lot for your detailed code review,it really helps me a lot,actually this is my first time taking part in open source activities,there are still many things for me to learn! |
…ngsphere/master * remotes/origin/master: mysql-proxy: skip admin for no-FROM multi-expression selects; add tests (apache#37391) Add CDCJobRunnerCleanerTest (apache#37398)
Fixes #37276.
Root cause: when not using a database and executing no-FROM multi-expression SELECT, query is routed to admin branch;
UnicastResourceShowExecutor rebuilds headers and rows from different sources, causing column-count mismatch and
IndexOutOfBoundsException in DatabaseAdminQueryProxyBackendHandler.
- Fix: routing-only. For normal no-FROM multi-expression SELECTs (non-sysvar, non-special functions), return Optional.empty() to use the
standard pipeline; keep sysvar/special functions/no-resource behavior unchanged. No silent truncate/padding, aligned with maintainer’s
suggestion.
- Tests: add a test to assert skipping admin for multi-expression no-FROM; add a regression test to assert multi sysvars still use
MySQLSystemVariableQueryExecutor.
- Impact: minimal, routing only; no behavior change for existing admin executors.
Before committing this PR, I'm sure that I have checked the following options:
./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.